-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve partial unification handling #9523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@milessabin This PR broke shapeless compilation as described in 9e7cc10, do you have the time to try to diagnose/minimize what's going on here? |
It looks like the sort of change which would be likely to create new ambiguities and I can imagine that breaking the I'm afraid I'm really not able to commit the time to look into this properly right now :-( |
Understood, thanks for taking a look. I'll look into this a bit more but since I'd like to get this in sooner rather than later (since it's a net improvement, it's potentially disruptive, and it's exercised by cats), I will probably have to comment out some of the code related to Pure in the branch of shapeless in our community build. |
test performance please |
performance test scheduled: 3 job(s) in queue, 1 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9523/ to see the changes. Benchmarks is based on merging with master (74ba21c) |
0e4a673
to
fa3c400
Compare
Not only can this prevent some constraints from being inferred, it can lead to the wrong constraints being inferred due to partial unification, see comment. This change broke one test, in i6565.scala we had: type Lifted[A] = Err | A extension [O, U](o: Lifted[O]) def map(f: O => U): Lifted[U] = ??? val error: Err = Err() lazy val ok: Lifted[String] = { point("a").map(_ => if true then "foo" else error) // now fails because error does not have type String } Because `isMatchingApply` can now deal with aliased types, when typing the lambda we get the constraint `U <: String` from `Lifted[U] <: Lifted[String]`, so `Err` is no longer a subtype of the expected result type of the lambda. This can be fixed in a number of ways: I changed the test to use `flatMap` instead of `map`, but one could also remove the expected type, or replace it by `Lifted[Lifted[String]` or `Err | String`. I think the new behavior is arguably better since using type aliases now gives you more control on how type inference proceeds, even if it means that some things that used to typecheck don't anymore. This change is also necessary to get shapeless to compile after the following commit which makes partial unification work in more situation.
#9504 is fixed, but the implicit issue is still here, however the issue goes away if the parameter is not marked by-name now (it didn't before), and I've been able to find a similar issue on master: #9568 |
So far, like Scala 2, given: Either[Int, String] <:< ?F[String] we were able to infer: ?F >: [X] =>> Either[Int, X] However, in the inverse situation: ?F[String] <:< Either[Int, String] Scala 2, but not Dotty, was able to infer: ?F <: [X] =>> Either[Int, X] This commit fixes this by generalizing the partial unification logic to be run both in `compareAppliedType2` and `compareAppliedType1`, this broke a few tests: - In `anykind.scala`, `kinder1` and `kinder2` became ambiguous, this is fixed by moving `kinder2` in a lower priority base trait. - One of the implicit search in tests/neg/i3452.scala now goes into an infinite loop, I've disabled it for now. The issue seems to be related to by-name implicits as the divergence checker works when the by-name is removed from one of the implicit parameter, I've added the non-by-name version in the same file. I've also been able to reproduce the same symptoms on master and filed scala#9568.
If `canInstantiate` fails, we fallback to `compareLower` which itself can fallback to `fourthTry`, so there's no need to fallback to `fourhTry` inside `canInstantiate` itself.
It turns out that neg/t2712-2.scala actually works in Dotty due to better type inference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I went through it in detail. It all makes sense to me.
* (even though the type constructor variable isn't actually unified but only | ||
* has one of its bounds constrained), for background see: | ||
* - The infamous SI-2712: https://github.com/scala/bug/issues/2712 | ||
* - The PR against Scala 2.12 implementing -Ypartial-unification: https://github.com/scala/scala/pull/5102 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice explanation!
Currently failing because it breaks shapeless.